Fix concurrent group access to prevent NullPointerException #10
Conversation
Closes #40368 Signed-off-by: vramik <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a fix for concurrent group access issues to prevent NullPointerExceptions in Keycloak's group management system. The changes span across multiple layers of the architecture:
-
Caching Layer Enhancement: In
CachedGroup.java, the@Overrideannotation is added to thegetRealm()method to ensure proper interface implementation compliance with theInRealminterface, improving code maintainability and compile-time verification. -
Group Adapter Null Safety: The
GroupAdapter.javafile receives a critical fix in thegetSubGroupsCount()method, where null-safety is added by checking if the model frommodelSupplier.get()is null before calling operations on it. This prevents NPEs in concurrent scenarios where groups might be deleted while other threads access them. -
GroupUtils Refactoring: The
GroupUtils.javaclass has thegroupMatchesSearchOrIsPathElementmethod removed as part of the broader refactoring. This appears to consolidate functionality and eliminate potentially redundant code paths. -
Comprehensive Test Coverage: A new concurrency test
createMultiDeleteMultiReadMulti()is added toGroupTest.javathat creates 100 groups, performs concurrent read operations in a background thread while deletion is happening, and verifies no exceptions occur. This serves as a regression test for the concurrent access fixes.
These changes work together to address race conditions in Keycloak's group caching and access mechanisms, particularly in Infinispan-based implementations where concurrent modifications can lead to inconsistent states.
Confidence score: 2/5
- This PR has significant null safety issues that could cause immediate production problems
- While the caching layer fix is solid, the GroupUtils.java file still contains multiple potential NullPointerExceptions that weren't addressed
- Files needing more attention:
services/src/main/java/org/keycloak/utils/GroupUtils.javacontains unaddressed NPE risks on lines 47 and 71
4 files reviewed, 3 comments
| GroupModel model = modelSupplier.get(); | ||
| return model == null ? null : model.getSubGroupsCount(); | ||
| } |
There was a problem hiding this comment.
style: This pattern differs from other methods like getSubGroupsStream() which call getDelegateForUpdate() when encountering null models. Consider consistency with the existing error handling pattern.
| new Thread(() -> { | ||
| while (!deletedAll.get()) { | ||
| try { | ||
| // just loading briefs | ||
| managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true); | ||
| } catch (Exception e) { | ||
|
|
||
| caughtExceptions.add(e); | ||
| } | ||
| } | ||
| }).start(); |
There was a problem hiding this comment.
logic: Thread is started but not joined, creating a race condition where the test might complete before the reader thread finishes executing
| new Thread(() -> { | |
| while (!deletedAll.get()) { | |
| try { | |
| // just loading briefs | |
| managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true); | |
| } catch (Exception e) { | |
| caughtExceptions.add(e); | |
| } | |
| } | |
| }).start(); | |
| Thread readerThread = new Thread(() -> { | |
| while (!deletedAll.get()) { | |
| try { | |
| // just loading briefs | |
| managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true); | |
| } catch (Exception e) { | |
| caughtExceptions.add(e); | |
| } | |
| } | |
| }); | |
| readerThread.start(); | |
| // delete groups | |
| groupUuuids.forEach(groupUuid -> { | |
| managedRealm.admin().groups().group(groupUuid).remove(); | |
| }); | |
| deletedAll.set(true); | |
| try { | |
| readerThread.join(5000); // Wait up to 5 seconds for thread to complete | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| fail("Test interrupted while waiting for reader thread"); | |
| } |
| @Test | ||
| public void createMultiDeleteMultiReadMulti() { | ||
| // create multiple groups | ||
| List<String> groupUuuids = new ArrayList<>(); |
There was a problem hiding this comment.
syntax: Variable name has typo: groupUuuids should be groupUuids
2 participants
Test 10